chore: add GH commit conventions check - #24

Does that mean the committer need to manually add the PR number to each commit title of a PR?

If so, I worry it's going to get really old really fast. It sounds annoying having to push the commit, create the PR to have a number attributed, then immediately amend all commits, and force-push again.

Essentially, I’m just replicating the practice we’ve followed for months on the sm: assigning a PR number to a commit, something we really ought to be doing consistently anyway.

Most PRs shouldn't involve many commits; usually, it's just 1 for fixes. If there is only a single commit in the PR, the script doesn't complain because GitHub can automatically add the ID.

Right now, we're adding a lot of features and tests because the project is recent; I suppose we could add a small utility script to make things easier and automate the PR tag. An option would be using merge commits, though there hasn't been much support for that in the past, except for major PR features. And another option: always use squash. On XO side, PRs are generally squashed, but I'm not entirely a fan of this solution either. 🙃

The discussion is open, and I’m happy to reach a compromise; in any case, differing opinions seem inevitable.

Just a quick reminder for all contributors and reviewers: anything currently pushed to the repo (even if merged and reviewed by a few people) can still be rediscussed later. Many elements are proposals, not set in stone.

EDIT: No solution on GH side: https://github.com/orgs/community/discussions/62118

GitHub already tracks the PRs that are associated with a commit. This is available from the web interface (when opening a commit), the GitHub CLI and it's basically just an API endpoint: https://docs.github.com/en/rest/commits/commits?apiVersion=2026-03-10#list-pull-requests-associated-with-a-commit.
I've been using this quite a lot to get back to the PR when looking at a commit, and this tend to work pretty well :)

The only drawback to this is if we rewrite the commit history and force-push, like we do in the sm repository. In that case, GitHub loses track of this association. I understand the reasons why we do it in that repository. I'd argue we should avoid force-pushing long-running branches in xcp-storage.

Most PRs shouldn't involve many commits; usually, it's just 1 for fixes. If there is only a single commit in the PR, the script doesn't complain because GitHub can automatically add the ID.

I like to split my PR commits a bit :o

Right now, we're adding a lot of features and tests because the project is recent; I suppose we could add a small utility script to make things easier and automate the PR tag.

I like this idea.

An option would be using merge commits, though there hasn't been much support for that in the past, except for major PR features. And another option: always use squash. On XO side, PRs are generally squashed, but I'm not entirely a fan of this solution either. 🙃

Not a fan of either solution too ;)

I just repushed a new action to update commit PR IDs using a simple GH message: /add-pr-id.
Should be launched after review.

Example:

Was there any discussion about using prek/pre-commit? I think this would be a great check to integrate as a git hook,

I still have a list of local checks to add. I was also considering this idea perhaps with some code adjustments, though I don't have a strong opinion on the matter; I realize it might cause some grumbling regarding temporary commits, conventions not respected on draft (though, of course, it's always possible to push without triggering the hooks).

I still have a list of local checks to add.

👍

I realize it might cause some grumbling regarding temporary commits, conventions not respected on draft

That's a good point. I was certain we could have pre-commit show only warning, which could have been an option here, but apparently not.

Between losing a bit of time writing a properly-formatted commit title (the body can always be filled out later while the commit is still being worked on), which you'd have to do at some point anyway, and a few seconds waiting for the CI, and if it fails: amend the commit, force-push it, clear the GitHub notification, delete the email notification; I personally prefer the former :)

But I'm all ears if there are differing opinions.

Do we want to also validate that there is a Signed-off-by line in the commit message footer, and that it has the same username and address as the commit author?

We'd probably need to handle multiple of those lines, and may want to check that it was indeed placed in the footer, and that there is a blank line between the body and the footer (but those two last checks sound a bit annoying to implement)...

I would prefer to avoid duplicating the DCO role on GitHub, not sure about advantages here.